Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken docstrings in bindings and minor API docs updates #920

Merged
merged 29 commits into from
Jul 24, 2023

Conversation

CalMacCQ
Copy link
Contributor

@CalMacCQ CalMacCQ commented Jul 7, 2023

Fix missing/broken docstrings in various places

This fixes the docstrings for RoundAngles, ZZPhaseToRz, Routingpass, CustomRoutingPass, Bit.to_list(), NoiseAwarePlacement.to_dict(), ZXDiagram.get_boundary() some stuff in pytket.pauli as well

This PR also updates the docs for pytket.qasm and fixes the misleading docstring for ZZPhaseToRz

There were some issues with my local build not matching the html pages built on CI but these seem to be resolved now

I have checked everything with sphinx==6.1.3 and it seems to work. This seems to be the verison used in CI.

See the issue #916

Screenshot 2023-07-21 at 19 06 53

@CalMacCQ CalMacCQ changed the title fix RenameQubitsPass docs Fix broken docstrings in bindings Jul 7, 2023
@CalMacCQ CalMacCQ marked this pull request as draft July 7, 2023 14:21
@CalMacCQ CalMacCQ marked this pull request as ready for review July 7, 2023 16:08
@CalMacCQ CalMacCQ requested a review from cqc-alec July 7, 2023 16:09

m.def(
"CnXPairwiseDecomposition", &CnXPairwiseDecomposition,
"Decompose CnX gates to 2-qubit gates and single qubit gates. "
"Decompose CnX gates to 2-qubit gates and single qubit gates."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This space was needed,

"Round angles to the nearest :math:`\\pi / 2^n`."
"\n\n:param n: precision parameter, must be >= 0 and < 32",
"\n\n:param only_zeros: if True, only round angles less than "
"Round angles to the nearest :math:`\\pi / 2^n`. "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't actually see what the problem was with this one.

@@ -93,7 +93,7 @@ This prints out a summary of readouts (the final values of the classical bits) a

Each pytket :py:class:`Backend` comes with its own default compilation method. This is a recommended sequence of optimisation passes to meet the requirements of the specific :py:class:`Backend`.

The following code snippet will show how to compile a circuit to run on an IBM device. This requires setting up IBM credentials (see `this page <https://cqcl.github.io/pytket-qiskit/api/index.html#access-and-credentials>`).
The following code snippet will show how to compile a circuit to run on an IBM device. This requires setting up IBM credentials (see the `credentials guide <https://cqcl.github.io/pytket-qiskit/api/index.html#access-and-credentials>`_).
Copy link
Contributor Author

@CalMacCQ CalMacCQ Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this to avoid the duplicate link labelled "this page". Fixes a sphinx warning

@CalMacCQ CalMacCQ requested a review from cqc-melf July 21, 2023 15:39
Copy link
Contributor

@cqc-melf cqc-melf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments, have you checked the CI build docs as well?

@@ -1,16 +1,24 @@
pytket.qasm
==================================
:py:class:`Circuit` objects can be converted to and from OpenQASM, although we do not support all operations.
In particular, we do not currently support:
In particular, we do not currently support the ability to interpret gates acting on a whole register in the OpenQASM style.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is true? I thought we can do this with classical ops?

Copy link
Contributor Author

@CalMacCQ CalMacCQ Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this line was there before. I just removed the first bullet point which outdated. I just removed the first bullet point and added the second bullet point to the paragraph above.

If this line is also outdated we should remove this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove both. It is a bit misleading from my understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this line

@@ -855,8 +859,12 @@ PYBIND11_MODULE(passes, m) {

m.def(
"ZZPhaseToRz", &ZZPhaseToRz,
"Converts ZZPhase gates with angle pi or -pi to two Rz gates with"
"angle pi.\n:return: a pass to convert ZZPhase gates to Rz");
"Converts all ZZPhase gates in a circuit with angle 1 or -1 (radians) "
Copy link
Contributor

@cqc-melf cqc-melf Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Converts all ZZPhase gates in a circuit with angle 1 or -1 (radians) "
"Converts all ZZPhase gates in a circuit with angle 1 or -1 (half turns) "

Copy link
Contributor Author

@CalMacCQ CalMacCQ Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 radian is a half turn. The angle convention is that the 1 refers to the number of half turns. I can say "half turns" if preferred

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think half turns would be good 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"Converts ZZPhase gates with angle pi or -pi to two Rz gates with"
"angle pi.\n:return: a pass to convert ZZPhase gates to Rz");
"Converts all ZZPhase gates in a circuit with angle 1 or -1 (radians) "
"into two Rz gates each with a parameter value of 1 (radians). "
Copy link
Contributor

@cqc-melf cqc-melf Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"into two Rz gates each with a parameter value of 1 (radians). "
"into two Rz gates each with a parameter value of 1 (half turns). "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"Converts all ZZPhase gates in a circuit with angle 1 or -1 (radians) "
"into two Rz gates each with a parameter value of 1 (radians). "
"ZZPhase gates with parameter values other than 1 or -1 "
"(radians) are left "
Copy link
Contributor

@cqc-melf cqc-melf Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(radians) are left "
"(half turns) are left "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -21,7 +21,7 @@ Example usage:
from pytket.circuit.display import get_circuit_renderer

circuit_renderer = get_circuit_renderer() # Instantiate a circuit renderer
circuit_renderer.set_render_options(zx_style=False) # Configure render options
circuit_renderer.set_render_options(zx_style=True) # Configure render options
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the renderer no longer uses ZX by default

@CalMacCQ CalMacCQ requested a review from cqc-melf July 21, 2023 17:38
@CalMacCQ CalMacCQ changed the title Fix broken docstrings in bindings Fix broken docstrings in bindings and minor API docs updates Jul 21, 2023
@CalMacCQ CalMacCQ merged commit 67b323d into develop Jul 24, 2023
34 checks passed
@CalMacCQ CalMacCQ deleted the fix/round_angles branch July 24, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants